fix(store/state): perform potentially blocking RocksDB access in a blocking context#2076
fix(store/state): perform potentially blocking RocksDB access in a blocking context#2076kkovaacs wants to merge 4 commits into
Conversation
…king task The account and nullifier trees may be backed by `RocksDB`, so tree access must not run on an async worker thread directly.
The account state forest may be backed by `RocksDB`, so access must not run on an async worker thread directly.
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Looks fine ito code, I'm just a bit concerned that we're using block_in_place? Should these be spawn_blocking?
| tokio::runtime::Handle::current() | ||
| .block_on(db_update_task)? |
There was a problem hiding this comment.
Should we be using block_on? I guess we're sort of saved because we know this will only happen once, and always sequentially?
There was a problem hiding this comment.
An alternative would be using a oneshot channel for explicit synchronization on the database commit -- that would avoid this ugly block_on() call here.
There was a problem hiding this comment.
I think we can live with these as is until we get proper write isolation.
I've considered that -- but it gets ugly because that way we need an additional |
…rocksdb-access-to-blocking-tasks
2 participants
This PR moves runtime access to RocksDB-backed store state structures onto Tokio’s blocking path via
block_in_place().State::innerandState::forestlock access.storage_map_key_cacheupdates on the normal async path since they do not touch the RocksDB-backed forest. Likewise,blockchainoperations are entirely in-memory, so those too are left in async context.Using
block_in_place()is required because usingspawn_blocking()would require the closure to be'static, which in turn would require having an additionalArcwrapper aroundinnerandforest.Closes #1969